Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle explicit conversions between enums and native integers #48041

Merged
merged 8 commits into from
Sep 30, 2020
Merged

Handle explicit conversions between enums and native integers #48041

merged 8 commits into from
Sep 30, 2020

Conversation

cston
Copy link
Member

@cston cston commented Sep 25, 2020 •
edited by jcouv
Loading

Fixes #48035

Test plan #38821

@cston cston marked this pull request as ready for review September 25, 2020 03:52
@cston cston requested a review from a team as a code owner September 25, 2020 03:52
@cston cston added this to the 16.8 milestone Sep 25, 2020
@cston cston mentioned this pull request Sep 25, 2020
Merged
@cston cston linked an issue Sep 25, 2020 that may be closed by this pull request
Error reported for explicit conversion from native integer to enum #48035
Closed
@cston cston removed a link to an issue Sep 25, 2020
Error reported for explicit conversion from native integer to enum #48035
Closed
This was referenced Sep 25, 2020
Closed
Closed
@cston cston requested a review from a team September 25, 2020 16:25
@jcouv
Copy link
Member

jcouv commented Sep 25, 2020 •
edited by cston
Loading 

        // SPEC: From sbyte, byte, short, ushort, int, uint, long, ulong, char, float, double, or decimal to any enum-type.

nit: should the comment be update, and/or the native ints spec? #Resolved

Refers to: src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs:1985 in 0dc6d46. [](commit_id = 0dc6d46, deletion_comment = False)

jcouv
jcouv approved these changes Sep 25, 2020
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks (iteration 3)

@jcouv jcouv self-assigned this Sep 25, 2020
@RikkiGibson RikkiGibson self-assigned this Sep 25, 2020
@cston
Copy link
Member Author

cston commented Sep 25, 2020 •
edited
Loading 

        // SPEC: From sbyte, byte, short, ushort, int, uint, long, ulong, char, float, double, or decimal to any enum-type.

Updated comment. And @Youssef1313 has updated the spec in dotnet/csharplang#3941.

In reply to: 699029430 [](ancestors = 699029430)

Refers to: src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs:1985 in 0dc6d46. [](commit_id = 0dc6d46, deletion_comment = False)

RikkiGibson
RikkiGibson approved these changes Sep 25, 2020
View reviewed changes
src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs Outdated
@@ -1841,12 +1845,16 @@ public static bool IsConstantNumericZero(ConstantValue value)
return value.Int32Value == 0;
case ConstantValueTypeDiscriminator.Int64:
return value.Int64Value == 0;
case ConstantValueTypeDiscriminator.NInt:
Copy link
Contributor

@RikkiGibson RikkiGibson Sep 25, 2020 •
edited by cston
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add labels to the Int32 and UInt32 sections instead of creating new switch sections for these? #Resolved

src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs Outdated
{
string source =
@"using static System.Console;
enum E { A = -1, B = 1 }
Copy link
Contributor

@RikkiGibson RikkiGibson Sep 25, 2020 •
edited by cston
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any need to test conversion of enums with various underlying types? #Resolved

@cston cston removed this from the 16.8 milestone Sep 28, 2020
@cston cston added this to the 16.8.P4 milestone Sep 28, 2020
@cston cston merged commit 1c155c5 into dotnet:release/dev16.8 Sep 30, 2020
@cston cston deleted the 48035 branch September 30, 2020 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RikkiGibson RikkiGibson RikkiGibson approved these changes

@jcouv jcouv jcouv approved these changes

Assignees

@RikkiGibson RikkiGibson

@jcouv jcouv

Labels
Area-Compilers Needs Shiproom Approval New Language Feature - Native Int
Projects
None yet
Milestone
16.8.P4
Development

Successfully merging this pull request may close these issues.
3 participants
@cston @jcouv @RikkiGibson